-
Notifications
You must be signed in to change notification settings - Fork 12.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cleanup markdown span handling #80244
Conversation
This comment has been minimized.
This comment has been minimized.
r? @ollie27 (rust-highfive has picked a reviewer for you, use r? to override) |
I'd be fine with merging this first; the MarkdownLink refactor in #77859 could probably be done on top of this. |
This comment has been minimized.
This comment has been minimized.
This function was unfortunate for several reasons: - It used `unsafe` because it wanted to tell whether a string came from the same *allocation* as another, not just whether it was a textual match. - It recalculated spans even though they were already available from pulldown - It sometimes *failed* to calculate the span, which meant it was always possible for the span to be `None`, even though in practice that should never happen. This commit has several cleanups: - Make the span required - Pass through the span from pulldown in the `HeadingLinks` and `Footnotes` iterators - Only add iterator bounds on the `impl Iterator`, not on `new` and the struct itself.
r? @bugadani |
This comment has been minimized.
This comment has been minimized.
@bors r+ Looks neat :) |
@bugadani: 🔑 Insufficient privileges: Not in reviewers |
Hmm I expected bors to respect the review request. I guess I assumed too much :) |
@bors r=bugadani Thanks for the review! |
📌 Commit 60d5567 has been approved by |
Cleanup markdown span handling 1. Get rid of `locate()` in markdown handling This function was unfortunate for several reasons: - It used `unsafe` because it wanted to tell whether a string came from the same *allocation* as another, not just whether it was a textual match. - It recalculated spans even though they were already available from pulldown - It sometimes *failed* to calculate the span, which meant it was always possible for the span to be `None`, even though in practice that should never happen. This has several cleanups: - Make the span required - Pass through the span from pulldown in the `HeadingLinks` and `Footnotes` iterators - Only add iterator bounds on the `impl Iterator`, not on `new` and the struct itself. 2. Remove unnecessary scope in `markdown_links` I recommend reading a single commit at a time. cc `@bugadani` - this will conflict with rust-lang#77859, I'll try to make sure that gets merged first.
Rollup of 11 pull requests Successful merges: - rust-lang#80159 (Add array search aliases) - rust-lang#80166 (Edit rustc_middle docs) - rust-lang#80170 (Fix ICE when lookup method in trait for type that have bound vars) - rust-lang#80171 (Edit rustc_middle::ty::TyKind docs) - rust-lang#80199 (also const-check FakeRead) - rust-lang#80211 (Handle desugaring in impl trait bound suggestion) - rust-lang#80236 (Use pointer type in AtomicPtr::swap implementation) - rust-lang#80239 (Update Clippy) - rust-lang#80240 (make sure installer only creates directories in DESTDIR) - rust-lang#80244 (Cleanup markdown span handling) - rust-lang#80250 (Minor cleanups in LateResolver) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
…llaumeGomez Revert "Cleanup markdown span handling" Reverts rust-lang#80244. This caused a diagnostic regression, originally it was: ``` warning: unresolved link to `std::process::Comman` --> link.rs:3:10 | 3 | //! [a]: std::process::Comman | ^^^^^^^^^^^^^^^^^^^^ no item named `Comman` in module `process` | = note: `#[warn(broken_intra_doc_links)]` on by default ``` but after that PR rustdoc now displays ``` warning: unresolved link to `std::process::Comman` --> link.rs:1:14 | 1 | //! Links to [a] [link][a] | ^^^ no item named `Comman` in module `process` | = note: `#[warn(broken_intra_doc_links)]` on by default ``` which IMO is much less clear. cc `@bugadani,` thanks for catching this in rust-lang#77859. r? `@GuillaumeGomez`
…llaumeGomez Revert "Cleanup markdown span handling" Reverts rust-lang#80244. This caused a diagnostic regression, originally it was: ``` warning: unresolved link to `std::process::Comman` --> link.rs:3:10 | 3 | //! [a]: std::process::Comman | ^^^^^^^^^^^^^^^^^^^^ no item named `Comman` in module `process` | = note: `#[warn(broken_intra_doc_links)]` on by default ``` but after that PR rustdoc now displays ``` warning: unresolved link to `std::process::Comman` --> link.rs:1:14 | 1 | //! Links to [a] [link][a] | ^^^ no item named `Comman` in module `process` | = note: `#[warn(broken_intra_doc_links)]` on by default ``` which IMO is much less clear. cc `@bugadani,` thanks for catching this in rust-lang#77859. r? `@GuillaumeGomez`
Cleanup markdown span handling, take 2 This PR includes the cleanups made in rust-lang#80244 except for the removal of `locate()`. While the biggest conceptual part in rust-lang#80244 was the removal of `locate()`, it introduced a diagnostic regression. Additional cleanup: - Use `RefCell` to avoid building two separate vectors for the links Work to do: - [ ] Decide if `locate()` can be simplified by assuming `s` is always in `md` - [ ] Should probably add some tests that still provide the undesired diagnostics causing rust-lang#80381 cc `@jyn514` This is the best I can do without patching Pulldown to provide multiple ranges for reference-style links. Also, since `locate` is probably more efficient than `rfind` (at least it's constant time), I decided to not check the link type and just cover every &str as it was before.
locate()
in markdown handlingThis function was unfortunate for several reasons:
unsafe
because it wanted to tell whether a string came fromthe same allocation as another, not just whether it was a textual match.
None
, even though in practice that should never happen.This has several cleanups:
HeadingLinks
andFootnotes
iteratorsimpl Iterator
, not onnew
and the struct itself.markdown_links
I recommend reading a single commit at a time.
cc @bugadani - this will conflict with #77859, I'll try to make sure that gets merged first.